Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Emulate effects of j.u.l.Logger.setLevel #3125

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Emulate effects of j.u.l.Logger.setLevel #3125

merged 3 commits into from
Nov 21, 2024

Conversation

ppkarwasz
Copy link
Contributor

Some libraries rely on j.u.l.Logger.getLevel returning the value that was set using j.u.l.Logger.setLevel.

This PR changes #2353 so that:

  • By default, the effective configuration of the logging backend is not modified by log4j-jul.
  • Calling setLevel modifies the return value of getLevel. Both these methods should not be used in user code, because the effective level of a logger should be checked with j.u.l.isLoggable instead. Therefore, I don't see any potential problems with these modifications.

Closes #3119

@ppkarwasz
Emulate effects of j.u.l.Logger.setLevel
ca06bc0 
Some libraries rely on `j.u.l.Logger.getLevel` returning the value that was set using `j.u.l.Logger.setLevel`.

This PR changes #2353 so that:

- By default, the **effective** configuration of the logging backend is not modified by `log4j-jul`.
- Calling `setLevel` modifies the return value of `getLevel`. Both these methods should **not** be used in user code, because the **effective** level of a logger should be checked with `j.u.l.isLoggable` instead. Therefore, I don't see any potential problems with these modifications.

Closes #3119
@ppkarwasz ppkarwasz mentioned this pull request Oct 27, 2024
Closed
@ppkarwasz
Copy link
Contributor Author

Anticipating other similar problems with the j.u.l.Logger methods, what do you think about implementing in term of "WARN + super" also the: setFilter, addHandler, removeHandler, setUseParentHandler? We can still keep the current implementation for setParent (throwing).

The setResourceBundle method, on the other hand, should probably be implemented differently: after it is called, we need to replace the message factory used by the JUL bridge with one the uses the specified bundle.

@ppkarwasz ppkarwasz mentioned this pull request Oct 30, 2024
Closed
2 tasks
ppkarwasz referenced this pull request in openjdk/nashorn Nov 21, 2024
@ppkarwasz @szegedi
8343449: Nashorn method handle debug logging breaks with log4j-jul
5e78947 
Reviewed-by: attila
@ppkarwasz ppkarwasz merged commit 52dbb47 into 2.x Nov 21, 2024
9 checks passed
@ppkarwasz ppkarwasz deleted the fix/2.x/3119_set_level_call_parent branch November 21, 2024 10:14
@ppkarwasz
Copy link
Contributor Author

@kangarko,

Can you check if the latest 2.25.0-SNAPSHOT solves your problem with Nashorn? The URL of our Maven snapshot repository is on our download page.

@kangarko
Copy link

@ppkarwasz appreciated it, unfortunately we rely on the Velocity software which uses said library, I found out the problem started since of this commit: PaperMC/Velocity@f2d6e14

I do not see them using log4j2, my question is can I do something on my end? I am essentially developing a plugin for velocity which can add libraries to Velocity classpath, so I think I have some options on my end too. Thanks!

@kangarko kangarko mentioned this pull request Nov 21, 2024
Open
@kangarko
Copy link

@ppkarwasz could you please provide nashorn-core snapshot, instead? Because production is affected since October, I could just use the snapshot for now. Many thanks.

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Nov 21, 2024 •
edited
Loading

@kangarko,

I am not a Nashorn maintainer, sorry.

PaperMC does use log4j-jul: see the libs.version.toml file. You need to bump the version to 2.25.0-SNAPSHOT and recompile.

@kangarko
Copy link

kangarko commented Nov 21, 2024 •
edited
Loading

Gotcha, thanks for the that, I have passed them the relevant information. I am unfortunately not a Paper maintainer either.

@ppkarwasz
Copy link
Contributor Author

ppkarwasz commented Nov 21, 2024 •
edited
Loading

@kangarko,

The workaround for this bug was provided by the reporter in this comment:

The solution for now is to set

-Dlog4j2.julLoggerAdapter=org.apache.logging.log4j.jul.CoreLoggerAdapter

You can de facto apply that right now on prod.

@kangarko
Copy link

@ppkarwasz Thank you, you made my day with this fix!!!

I will add a note to our customers so that they can adjust their server system properties for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

log4j-jul adapter makes code throws NPEs
2 participants
@ppkarwasz @kangarko